Add react router links to Breadcrumbs #59

VictoriaBeilsten-Edmands · 2025-05-02T09:19:15Z

Adds LinkComponent prop for page routing. Closes #56.
Reorganises components to mirror Storybook component structure.
Fixes type issue with image in Footer and Navbar.
I had to add yet another config file to get the tests running due to an issue with react-router-dom v7. See here and here.

akademy

This didn't work "out of the box" for me when I used it in my external app, because I didn't have <BrowserRouter> added.

I'm not sure how to handle this. Maybe we could add it to some-kind of <SciReactProvider/> (which could also embed the theme) so that just the one line can be added in external apps. Something like:

const SciReactUIProvider = function ({children}) {
  return (
    <ThemeProvider>
      <BrowserRouter>
        {children}
      </BrowserRouter>
    </ThemeProvider>
  );
};

The alternative is to just add it to the docs but I like to hide, behind the scenes, as much as we can What do you think?

package.json

src/components/Breadcrumbs.tsx

VictoriaBeilsten-Edmands · 2025-05-06T09:52:14Z

We could create the SciReactUIProvider which would leave the Breadcrumbs component unchanged and would leave the option for people to use e.g. RouterProvider in place of BrowserProvider together withe the ThemeProvider. I should still update the docs with something.

akademy · 2025-05-06T10:01:34Z

I thought we still had to change Breadcrumbs to use ReactRouter's Link though. Or does it just pick up MUI's version?

VictoriaBeilsten-Edmands · 2025-05-06T11:03:04Z

My comment was unclear! I mean leave the Breadcrumbs component unchanged from how it is in this PR.

VictoriaBeilsten-Edmands · 2025-05-06T14:16:30Z

I've added SciReactUIProvider and updated the docs to replace ThemeProvider with this.

akademy

Looks good. Just a few minor suggestions.

changelog.md

src/themes/SciReactUIProvider.tsx

akademy · 2025-05-07T14:09:27Z

The SciReactUIProvider doesn't really fit in the "Themes" folder... but I don't really know the best place to put it...

akademy · 2025-05-07T14:15:05Z

Also, we need an export in index.ts:

export * from "./themes/SciReactUIProvider";

VictoriaBeilsten-Edmands · 2025-05-07T14:18:03Z

The SciReactUIProvider doesn't really fit in the "Themes" folder... but I don't really know the best place to put it...

I was a bit stuck on where to put it.

akademy · 2025-05-08T09:03:03Z

The SciReactUIProvider doesn't really fit in the "Themes" folder... but I don't really know the best place to put it...

I was a bit stuck on where to put it.

Maybe it's time to reorganise the component folder... controls, sections, general ?

MattPrit · 2025-05-09T11:49:02Z

Are we sure we want to add a dependency to a specific routing library? Or has Diamond made a decision on which we should use?

VictoriaBeilsten-Edmands · 2025-05-12T12:51:57Z

Are we sure we want to add a dependency to a specific routing library? Or has Diamond made a decision on which we should use?

As far as I know there's no Diamond-wide decision on which we should use. I do think react-router is a good choice and what we should use here. I don't see a reason to diverge with multiple options.

gfrn · 2025-05-13T14:23:02Z

Are we sure we want to add a dependency to a specific routing library? Or has Diamond made a decision on which we should use?

As far as I know there's no Diamond-wide decision on which we should use. I do think react-router is a good choice and what we should use here. I don't see a reason to diverge with multiple options.

I think allowing people to do component "impersonation" in the breadcrumbs component is not going to lead to diversion, it would be useful for the following reasons:

If someone decides to use a non-BrowserRouter provider for react-router, then they are able to do so
We are not locked to one specific version of react-router, and being able to decouple updating react-router with updating sci-react-ui is really useful, because it might not be trivial for all people (especially if they are using/want to use version specific features, and going from v6 to v7 was not easy if you're using advanced features)
Not everyone is going to use react-router anyway, some people might elect to use simpler native browser navigation, or in my case, Next's own native navigation
If we choose to move to a different routing library in the future (like Tanstack Router), the switch is much easier, and we're not tying our UI to a routing library
This decreases the number of peer/direct dependencies, so when security updates come out, people won't have to wait on us updating sci-react-ui, so this is a safer option
It's not a "free" win, but it's at least technically feasible, and MUI already does this for plenty of components (in fact, this is a feature which is being used in this PR)

VictoriaBeilsten-Edmands · 2025-05-13T14:57:11Z

Are we sure we want to add a dependency to a specific routing library? Or has Diamond made a decision on which we should use?

As far as I know there's no Diamond-wide decision on which we should use. I do think react-router is a good choice and what we should use here. I don't see a reason to diverge with multiple options.

I think allowing people to do component "impersonation" in the breadcrumbs component is not going to lead to diversion, it would be useful for the following reasons:

* If someone decides to use a non-`BrowserRouter` provider for `react-router`, then they are able to do so

* We are not locked to one specific version of `react-router`, and being able to decouple updating `react-router` with updating `sci-react-ui` is really useful, because it might not be trivial for all people (especially if they are using/want to use version specific features, and going from v6 to v7 was not easy if you're using advanced features)

* Not everyone is going to use `react-router` anyway, some people might elect to use simpler native browser navigation, or in my case, Next's own native navigation

* If we choose to move to a different routing library in the future (like Tanstack Router), the switch is much easier, and we're not tying our UI to a routing library

* This decreases the number of peer/direct dependencies, so when security updates come out, people won't have to wait on us updating `sci-react-ui`, so this is a safer option

* It's not a "free" win, but it's at least technically feasible, and MUI already does this for plenty of components (in fact, this is a feature which is being used in this PR)

I could change the Breadcrumbs component and pass a LinkComponent e.g.

interface BreadcrumbsProps {
  path: string | string[] | CustomLink[];
  linkComponent: React.ElementType
  rootProps?: PaperProps;
  muiBreadcrumbsProps?: Mui_BreadcrumbsProps;
}

Then this can be used with whatever routing library e.g. Link from react-router-dom/ next etc.
We wouldn't have the SciReactUIProvider if we want to keep the dependency out of the library, so that would be removed.

VictoriaBeilsten-Edmands · 2025-05-13T14:58:07Z

The SciReactUIProvider doesn't really fit in the "Themes" folder... but I don't really know the best place to put it...

I was a bit stuck on where to put it.

Maybe it's time to reorganise the component folder... controls, sections, general ?

Shall we go with component/controls and component/navigation to mirror the storybook. I quite like those categories.

gfrn · 2025-05-14T09:44:04Z

Are we sure we want to add a dependency to a specific routing library? Or has Diamond made a decision on which we should use?

As far as I know there's no Diamond-wide decision on which we should use. I do think react-router is a good choice and what we should use here. I don't see a reason to diverge with multiple options.

I think allowing people to do component "impersonation" in the breadcrumbs component is not going to lead to diversion, it would be useful for the following reasons:
* If someone decides to use a non-`BrowserRouter` provider for `react-router`, then they are able to do so

* We are not locked to one specific version of `react-router`, and being able to decouple updating `react-router` with updating `sci-react-ui` is really useful, because it might not be trivial for all people (especially if they are using/want to use version specific features, and going from v6 to v7 was not easy if you're using advanced features)

* Not everyone is going to use `react-router` anyway, some people might elect to use simpler native browser navigation, or in my case, Next's own native navigation

* If we choose to move to a different routing library in the future (like Tanstack Router), the switch is much easier, and we're not tying our UI to a routing library

* This decreases the number of peer/direct dependencies, so when security updates come out, people won't have to wait on us updating `sci-react-ui`, so this is a safer option

* It's not a "free" win, but it's at least technically feasible, and MUI already does this for plenty of components (in fact, this is a feature which is being used in this PR)
I could change the Breadcrumbs component and pass a LinkComponent e.g.
interface BreadcrumbsProps {
  path: string | string[] | CustomLink[];
  linkComponent: React.ElementType
  rootProps?: PaperProps;
  muiBreadcrumbsProps?: Mui_BreadcrumbsProps;
}
Then this can be used with whatever routing library e.g. Link from react-router-dom/ next etc. We wouldn't have the SciReactUIProvider if we want to keep the dependency out of the library, so that would be removed.

I think this is a nice solution (to allow users to set both the link component and the prop each breadcrumb would use in that link component), and removing the provider surely would make things easier in the future!

Add react router links to Breadcrumbs

97f9a4b

VictoriaBeilsten-Edmands requested a review from akademy May 2, 2025 09:20

akademy requested changes May 5, 2025

View reviewed changes

package.json Outdated Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

src/components/Breadcrumbs.tsx Outdated Show resolved Hide resolved

src/components/Breadcrumbs.tsx Outdated Show resolved Hide resolved

Add SciReactUIProvider to use ThemeProvider and BrowserRouter

efb296e

VictoriaBeilsten-Edmands force-pushed the vbe/breadcrumbs-react-router branch from 0aaff20 to efb296e Compare May 6, 2025 14:13

akademy requested changes May 7, 2025

View reviewed changes

changelog.md Outdated Show resolved Hide resolved

src/themes/SciReactUIProvider.tsx Outdated Show resolved Hide resolved

VictoriaBeilsten-Edmands force-pushed the vbe/breadcrumbs-react-router branch 3 times, most recently from e9e653a to 6ad9e23 Compare May 15, 2025 14:54

Use LinkComponent prop in Breadcrumbs & reorganise components

1a36fe7

VictoriaBeilsten-Edmands force-pushed the vbe/breadcrumbs-react-router branch from 6ad9e23 to 1a36fe7 Compare May 15, 2025 15:00

Keep static link behaviour on Breadcrumbs when no linkComponent passed

c41370a

akademy approved these changes May 20, 2025

View reviewed changes

VictoriaBeilsten-Edmands merged commit ca9e2f8 into main May 20, 2025
1 check passed

VictoriaBeilsten-Edmands deleted the vbe/breadcrumbs-react-router branch May 20, 2025 15:01

Add react router links to Breadcrumbs #59

Add react router links to Breadcrumbs #59

Uh oh!

Conversation

VictoriaBeilsten-Edmands commented May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akademy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

VictoriaBeilsten-Edmands commented May 6, 2025

Uh oh!

akademy commented May 6, 2025

Uh oh!

VictoriaBeilsten-Edmands commented May 6, 2025

Uh oh!

VictoriaBeilsten-Edmands commented May 6, 2025

Uh oh!

akademy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

akademy commented May 7, 2025

Uh oh!

akademy commented May 7, 2025

Uh oh!

VictoriaBeilsten-Edmands commented May 7, 2025

Uh oh!

akademy commented May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MattPrit commented May 9, 2025

Uh oh!

VictoriaBeilsten-Edmands commented May 12, 2025

Uh oh!

gfrn commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VictoriaBeilsten-Edmands commented May 13, 2025

Uh oh!

VictoriaBeilsten-Edmands commented May 13, 2025

Uh oh!

gfrn commented May 14, 2025

Uh oh!

Uh oh!

Uh oh!

VictoriaBeilsten-Edmands commented May 2, 2025 •

edited

Loading

akademy commented May 8, 2025 •

edited

Loading

gfrn commented May 13, 2025 •

edited

Loading